Skip to content

Conversation

@sungshik
Copy link
Contributor

@sungshik sungshik commented Feb 24, 2025

What

This PR generalizes the signature of internal event handlers from Consumer<WatchEvent> to BiConsumer<EventHandlingWatch, WatchEvent>. Thus, when internal event handlers are called, they also get access to the watch that dispatched the event.

This change affects only the internals of the library (currently only the JDK...Watch classes, but later also the macOS code). The end-user API still expects Consumer<WatchEvent> (or WatchEventListener).

Why

Imagine an overflow event happens, so the directory needs to be rescanned. Before this PR, the event handler of the watch could look something like this (simplified code, just to make the point):

class FooWatch implements EventHandlingWatch {

  ...

  @override
  public void handleEvent(WatchEvent event) {
    if (event.getKind() == WatchEvent.Kind.OVERFLOW) {
      var events = rescanDirectory(); // Returns a collection of rescanning events (CREATED or MODIFIED)
      for (var e : events) {
        // Recursively call this method for each of the rescanning events
        handleEvent(e);
      }
    }

    // Run the user-defined event handler
    this.eventHandler.accept(event);
  }
}

The point is that, to auto-handle overflow events, the auto-handler must access a watch to dispatch rescanning events (i.e., call the watch's handleEvent method). There are several alternatives to achieve this, including:

  1. Define auto-handlers inside the watch class -- This is essentially what currently happens in JDKRecursiveDirectoryWatch
  2. Define auto-handlers outside the watch class; pass a watch class reference when the auto-handler is constructed
  3. Define auto-handlers outside the watch class; pass a watch class reference when the auto-handler is called

The problem with alternative 1 is that each auto-handler class should be usable with multiple watch classes (otherwise, we're potentially duplicating a lot of code). The problem with alternative 2 is that each auto-handler instance should be usable with multiple watch instances (otherwise, we're potentially duplicating a lot of handler objects that do essentially the same thing).

Thus, this PR implements alternative 3. It enables:

  • Defining auto-handler code completely independent of the watch classes (as long as they implement the EventHandlingWatch interface, which has method handleEvent)
  • Construct a single auto-handler that can be (re)used by multiple watches

(Usages of these features are demonstrated in the next PR.)

@sungshik sungshik mentioned this pull request Feb 24, 2025
7 tasks
Base automatically changed from improved-overflow-support/richer-active-watch to improved-overflow-support-main February 24, 2025 11:20
@codecov
Copy link

codecov bot commented Feb 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.6%. Comparing base (8d4dd06) to head (d52cfeb).
Report is 6 commits behind head on improved-overflow-support-main.

Additional details and impacted files
@@                       Coverage Diff                        @@
##             improved-overflow-support-main     #21   +/-   ##
================================================================
  Coverage                              80.6%   80.6%           
- Complexity                               95      97    +2     
================================================================
  Files                                    12      12           
  Lines                                   428     428           
  Branches                                 41      41           
================================================================
  Hits                                    345     345           
  Misses                                   61      61           
  Partials                                 22      22           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sungshik sungshik marked this pull request as ready for review February 24, 2025 12:16
@sungshik sungshik requested a review from DavyLandman February 24, 2025 12:17
Copy link
Member

@DavyLandman DavyLandman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally dislike BiConsumers (as they start to hint at the fact it should have been a class/interface), but I'm okay with this.

@sungshik sungshik merged commit a712a3a into improved-overflow-support-main Feb 24, 2025
13 checks passed
@sungshik sungshik deleted the improved-overflow-support/event-handlers-with-active-watch branch February 24, 2025 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants